[GCS] Move node resource info from client table to resource table by kfstorm · Pull Request #5050 · ray-project/ray

kfstorm · 2019-06-27T13:54:17Z

What do these changes do?

This is the continuation of #4911. Previously node resource information is stored in client table. This PR moves it to resource table. This change makes client table more lightweight and simplifies the merge logic of client table because RES_CREATEUPDATE and RES_DELETE are removed from code. Resource changes for a living node no need to update client table any more.

Related issue number

Linter

I've run scripts/format.sh to lint the changes in this PR.

AmplabJenkins · 2019-06-27T16:58:07Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14936/
Test PASSed.

AmplabJenkins · 2019-06-28T09:51:06Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14952/
Test PASSed.

AmplabJenkins · 2019-06-28T20:02:10Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14968/
Test PASSed.

zhijunfu · 2019-07-06T07:51:00Z

@raulchen @romilbhardwaj Any comments on this one? thanks

raulchen · 2019-07-06T07:59:59Z

Oops, I forgot to review this one. Will take a look soon.

raulchen · 2019-07-07T03:28:23Z

java/runtime/src/main/java/org/ray/runtime/gcs/GcsClient.java

-        }
        NodeInfo nodeInfo = new NodeInfo(
-            clientId, data.getNodeManagerAddress(), true, resources);
+            clientId, data.getNodeManagerAddress(), true, new HashMap<>());


nit, prefer using ImmutableMap.of()

raulchen · 2019-07-07T03:28:34Z

java/runtime/src/main/java/org/ray/runtime/gcs/GcsClient.java

        Preconditions.checkState(data.getEntryType() == EntryType.DELETION);
        NodeInfo nodeInfo = new NodeInfo(clientId, clients.get(clientId).nodeAddress,
-            false, clients.get(clientId).resources);
+            false, new HashMap<>());


raulchen · 2019-07-07T03:34:24Z

java/runtime/src/main/java/org/ray/runtime/gcs/RedisClient.java

+    try (Jedis jedis = jedisPool.getResource()) {
+      return jedis.hgetAll(key);
+    }
+


remove blank line.

All the other methods in this class have a blank line at the end. Should I remove the blank lines of all methods?

better also remove them, thanks.

raulchen · 2019-07-07T03:35:34Z

python/ray/gcs_utils.py

    "TablePubsub",
    "Task",
    "TaskTableData",
+    "RayResource",


maybe rename RayResource to ResourceTableData?

src/ray/gcs/tables.cc

raulchen · 2019-07-07T03:43:08Z

src/ray/raylet/node_manager.cc

+    // 1) Client cache is up to date.
+    // 2) Lookup resource table after subscribe.
+    io_service_.post([this]() {
+      // Fetch resource info for all clients and update cluster resource map.


Maybe I'm misunderstanding something.
Why do we need to fetch resource info for all clients here?
This is the callback of one node being added. Why not just fetch that node?

It seems like this lambda is called only when the callback is for the local (self) raylet. This is the case when the raylet has been newly instantiated and has no resource state of other raylets. This lambda populates the resources in the raylet by querying the resource table and calling ResourceCreateUpdated on the resources of all nodes. Is that correct @kfstorm?

I see. thanks.

@romilbhardwaj You are right. Previously the resource information is stored in the client table, so a client table notification carries resource update information. Now the two kinds of data are separated, we need some extra logic to make sure local resource information is up to date.

I updated the code to lookup resources for only one client at the end of ClientAdded.

src/ray/raylet/node_manager.cc

raulchen · 2019-07-07T03:46:14Z

src/ray/raylet/node_manager.cc

-    worker = worker_pool_.GetRegisteredDriver(client);
+    std::unordered_map<std::string, std::shared_ptr<gcs::RayResource>> data_map;
+    auto ray_resource = std::make_shared<gcs::RayResource>();
+    ray_resource->set_resource_name(resource_name);


I think the resource_name is redundant. Because the resource table is a Hash. The key is already the resource name.

You mean we remove the resource_name field from RayResource structure? BTW, We can't replace RayResource structure with double because the Hash template class requires a protobuf structure as the value type to be able to serialize it.

yes, we can have a ResourceTableData that only has a double field.

raulchen · 2019-07-07T03:47:58Z

src/ray/raylet/node_manager.cc

+    // Delay execution to make sure,
+    // 1) Client cache is up to date.
+    // 2) Lookup resource table after subscribe.
+    io_service_.post([this]() {


how to guarantee that this lambda will run after the client cache is up to date?

The execution of RequestNotifications against the client table will push all entries to raylet. Raylet will handle a single notification with multiple client entries and add alive and dead clients to the client cache. So here we lookup resources asynchronously to ensure all client entries are added to the client cache before the resource lookup operation.

This change is confusing. I updated the code to lookup resources for only one client at the end of ClientAdded.

romilbhardwaj

Thanks for submitting this PR! I've added some comments

romilbhardwaj · 2019-07-07T20:34:18Z

python/ray/state.py

    return [node_info[client_id] for client_id in ordered_client_ids]


+def _parse_resource_table(redis_client, client_id):


Nit: Considering client_id is stored/used in hex at most places in state.py and elsewhere, would it be more consistent to have the client_id argument as hex here and convert it to binary inside this method, rather than invoking it as _parse_resource_table(redis_client, ray.utils.hex_to_binary(client_id))?

Sure. Sorry that I'm not familiar with state.py.

romilbhardwaj · 2019-07-07T20:35:07Z

python/ray/state.py

+
+    Args:
+        redis_client: A client to the primary Redis shard.
+        client_id: The client ID of the node.


Would be good to clarify the encoding (hex/binary) of the client_id here

romilbhardwaj · 2019-07-07T21:27:51Z

src/ray/raylet/node_manager.cc

+    // 1) Client cache is up to date.
+    // 2) Lookup resource table after subscribe.
+    io_service_.post([this]() {
+      // Fetch resource info for all clients and update cluster resource map.


It seems like this lambda is called only when the callback is for the local (self) raylet. This is the case when the raylet has been newly instantiated and has no resource state of other raylets. This lambda populates the resources in the raylet by querying the resource table and calling ResourceCreateUpdated on the resources of all nodes. Is that correct @kfstorm?

romilbhardwaj · 2019-07-07T21:35:55Z

src/ray/raylet/node_manager.cc

+    // Delay execution to make sure,
+    // 1) Client cache is up to date.
+    // 2) Lookup resource table after subscribe.
+    io_service_.post([this]() {


Perhaps I'm missing something, but instead of using this lambda, why can't we update the resources of the remote clients at line 412 when they're added to the local raylet? Even if client cache is not up to date, subsequent notifications should handle update/delete in the local raylet.

I just want to make sure the remote client is already registered in local raylet when receiving a resource update notification. Thus can prevent potential bugs in the resource update notification handling code path.

After discussed with @raulchen, we think that it's OK to put the lookup at the end of ClientAdded and only lookup resources for one client. So I updated the code as you suggested to avoid the lambda.

raulchen · 2019-07-08T04:32:54Z

src/ray/raylet/monitor.cc

          }
          if (!marked) {
            RAY_CHECK_OK(gcs_client_.client_table().MarkDisconnected(client_id));
+            // Remove all resources of this client from GCS


Maybe sometimes we'll want query the resources of a dead node. I think we can keep it. it won't waste too much memory.

raulchen · 2019-07-08T04:34:27Z

src/ray/raylet/node_manager.cc

+    // 1) Client cache is up to date.
+    // 2) Lookup resource table after subscribe.
+    io_service_.post([this]() {
+      // Fetch resource info for all clients and update cluster resource map.


I see. thanks.

AmplabJenkins · 2019-07-10T09:56:38Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15268/
Test PASSed.

AmplabJenkins · 2019-07-10T10:08:21Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1578/
Test FAILed.

AmplabJenkins · 2019-07-10T10:22:32Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15269/
Test PASSed.

AmplabJenkins · 2019-07-10T10:34:20Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1579/
Test FAILed.

AmplabJenkins · 2019-07-10T12:42:32Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1584/
Test FAILed.

AmplabJenkins · 2019-07-10T13:24:33Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15274/
Test PASSed.

src/ray/gcs/tables.cc

kfstorm added 11 commits June 14, 2019 14:13

Temp commit

b3c36c8

temp commit

14034d9

Merge remote-tracking branch 'origin/master' into dynamic_resource_hash

7b06388

finish

78ac059

lint

df08658

finish

66c5170

Merge remote-tracking branch 'origin/master' into dynamic_resource_hash

40d85ea

lint

6f74075

Minor update

879c408

Add comment

9c81b67

lint

5f42f4f

kfstorm added 2 commits June 28, 2019 14:32

Merge remote-tracking branch 'origin/master' into dynamic_resource_hash

1e404e6

Fix merge

1636b37

Fix typo

411f22b

raulchen requested review from raulchen and romilbhardwaj June 30, 2019 03:46

raulchen reviewed Jul 7, 2019

View reviewed changes

romilbhardwaj reviewed Jul 7, 2019

View reviewed changes

raulchen reviewed Jul 8, 2019

View reviewed changes

kfstorm added 2 commits July 10, 2019 12:41

Merge remote-tracking branch 'origin/master' into dynamic_resource_hash

199a2fb

Address comments

717c527

Address comments about resource table lookup

a720fa5

Address comments

3113402

raulchen approved these changes Jul 11, 2019

View reviewed changes

src/ray/gcs/tables.cc Outdated Show resolved Hide resolved

Update tables.cc

5298165

raulchen merged commit 43b6513 into ray-project:master Jul 11, 2019

raulchen deleted the dynamic_resource_hash branch July 11, 2019 05:17

kfstorm mentioned this pull request Jul 11, 2019

Replace ImmutableMap.of() with new HashMap<>() in GcsClient.java #5171

Merged

1 task

		return [node_info[client_id] for client_id in ordered_client_ids]


		def _parse_resource_table(redis_client, client_id):

Conversation

kfstorm commented Jun 27, 2019

What do these changes do?

Related issue number

Linter

Uh oh!

AmplabJenkins commented Jun 27, 2019

Uh oh!

AmplabJenkins commented Jun 28, 2019

Uh oh!

AmplabJenkins commented Jun 28, 2019

Uh oh!

zhijunfu commented Jul 6, 2019

Uh oh!

raulchen commented Jul 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romilbhardwaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment